-
Notifications
You must be signed in to change notification settings - Fork 84
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: whaught The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good - let me know when you want a full review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good - let me know when you want a full l-g-t-m
/hold still testing that oob code verification works on the server |
/hold cancel Should all be working |
@@ -36,6 +39,13 @@ func (err *ErrorDetails) Error() string { | |||
return err.Err | |||
} | |||
|
|||
func (err *ErrorDetails) Is(target error) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you should need this? It also doesn't guarantee that the error is an ErrorDetails because it could be wrapped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the static ones up top, but they don't match unless it's a complete match and I also specify the http code for them. I was hoping to match only on this error enum firebase gives back
} | ||
|
||
var m map[string]string | ||
if err := json.Unmarshal(b, &m); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to handle the err case here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still want this to be understood as success if we fail to parse the body of the response
Co-authored-by: Seth Vargo <seth@sethvargo.com>
Co-authored-by: Seth Vargo <seth@sethvargo.com>
/lgtm |
Proposed Changes